feat: evidence-grade NIM discovery + all-modality cost-quality benchmark - #90
feat: evidence-grade NIM discovery + all-modality cost-quality benchmark#90seonghobae wants to merge 200 commits into
Conversation
…ark (#86) Optional stdlib-only benchmark harness (contextual_orchestrator/nim_benchmark.py): - Dynamic catalog discovery from the OpenAI-compatible GET /v1/models — no hard-coded inventory; deduplicated, sorted (response-order-drift immune), with machine-readable duplicate/invalid hygiene lists. - Capability probes for every contract NIM can host: chat completions, text completions, Responses API, embeddings, image understanding, video understanding, omni-style audio understanding, audio transcription, and audio speech — omni_capable derived, skipped probes always carry a machine-readable reason, bounded concurrency under one shared hard request budget. - Fair policy comparison on a locked task split: per-worker direct baselines (source of best-single-worker-in-hindsight), route_once, conduct capped at five steps, cheapest-eligible-worker; identical scorers, caps, timeouts, and token budgets across systems. - Honest cost accounting: actual cost 0 while the hosted catalog is free; hypothetical paid cost only from an explicit versioned pricing scenario, "unknown" otherwise; the two never mix. - Paired-bootstrap CIs, quality-latency and quality-hypothetical-cost Pareto frontiers, full provenance (git SHA, run id, catalog/manifest/ pricing hashes, parameters), schema-validated JSON/CSV/Markdown artifacts with a secret-leak refusal guard. - Fail closed: missing KV credential (NVIDIA_NIM_API_KEY, bootstrap env->KV only, never argv), incomplete discovery, exceeded budget, missing provenance, schema violations. - Deterministic --dry-run drives the whole pipeline against an in-process synthetic provider covering every modality class — zero network, byte-identical artifacts. - Tests: 100% statement+branch coverage of the new module, adversarial cases (malformed catalogs, duplicate ids, non-finite tokens/costs, rate limits, timeouts, order drift, secret redaction); new fuzz seam (Hypothesis + Atheris) for the catalog parser. - CI: manual + conservative monthly scheduled workflow, single-flight concurrency, hard budgets, pinned actions, 90-day artifact retention. - Docs: docs/nim_benchmark.md, architecture/tracks pointers, HELM (arXiv:2211.09110) added to docs/papers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughNVIDIA NIM 비용·품질 벤치마크 하네스를 추가했습니다. 카탈로그 탐색, modality 검증, 정책 평가, 비용·증거 검증, 결정적 dry-run, live CI 실행, artifact 생성 및 품질 테스트를 포함합니다. ChangesNIM 벤치마크 기능과 검증
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant GitHubActions
participant NIMBenchmark
participant NIMProvider
participant ArtifactStore
Operator->>GitHubActions: dry-run 또는 live 실행 요청
GitHubActions->>NIMBenchmark: 고정된 예산·manifest·provenance 전달
NIMBenchmark->>NIMProvider: 카탈로그 조회 및 capability probe
NIMProvider-->>NIMBenchmark: 모델·probe 결과 반환
NIMBenchmark->>NIMProvider: 평가 task 요청 전송
NIMProvider-->>NIMBenchmark: 응답·usage 반환
NIMBenchmark->>ArtifactStore: JSON·CSV·Markdown artifact 업로드
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Exact-head maintainer audit of
Do not mark this PR ready or resolve these findings from stale checks. Re-run all required workflows and independent review on the exact repaired head. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Blocking security finding
CRITICAL — benchmark HTTPS transport reintroduces DNS-rebinding SSRF
require_public_https_endpoint() validates one DNS answer, but build_default_transport() then calls urllib.request.urlopen(), which resolves the hostname again. The validated_hosts cache widens the gap by skipping validation on later calls. A provider hostname can therefore pass public-address validation and later connect to loopback, private, RFC 6598, link-local, or otherwise non-global infrastructure. Redirects are also delegated to urllib's default handler, so credentials may be forwarded to an unvalidated destination.
This is the same TOCTOU class repaired by PR #76. Keep this PR Draft until #76 is in main, then reuse or generalize its DNS-pinned transport so the socket dials only validation-time public IPs while preserving the original hostname for HTTP authority, TLS SNI, and certificate verification. Reject redirects, bypass environment proxies, require is_global, clear stale pins before each validation, and deterministically close failed sockets/responses.
Required regression evidence:
- no transport-time DNS re-resolution;
- public IPv4/IPv6 pinning and hostname/SNI preservation;
- RFC 6598/private/loopback/link-local/multicast/reserved/unspecified rejection;
- redirect rejection with no authorization propagation;
- approved-IP fallback and all-address failure;
- exact-head statement/branch coverage and full repository checks.
Do not merge based on the current local 100% claim; it does not cover this security invariant.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review — still blocked
The latest head addresses the three audit themes, but the implementation is not yet mergeable.
1. HIGH — production security behavior is coupled to a test monkeypatch
nim_benchmark_hardening._build_secure_transport() deliberately falls back to urllib.request.urlopen() whenever that global has been replaced. This keeps the old offline tests green by changing production behavior instead of replacing the obsolete tests. It reintroduces the hostname-resolving, proxy-aware, redirect-capable path that this repair is supposed to eliminate, and requires a nosemgrep waiver on the exact sink.
Remove the compatibility branch entirely. Rewrite the transport tests to inject the pinned connection/resolver seam and prove the real direct transport. Do not let test instrumentation select a less-secure production path.
2. HIGH — the new 531-line hardening module has no direct regression suite
The current tests/test_nim_benchmark.py still asserts the old urlopen behavior and contains no evidence for EqualBudgetModelClient, expiry enforcement, actual-cost evidence validation, configured/observed budget fields, or the installed wrappers. A 100% repository claim cannot be accepted while the newly shipped module and its branches are not directly exercised.
Add behavior tests that fail without each contract, then prove exact-head statement and branch coverage at 100% for both nim_benchmark.py and nim_benchmark_hardening.py, plus 100% public docstrings.
3. HIGH — cited cost source does not support the recorded claim
ACTUAL_COST_EVIDENCE names https://docs.nvidia.com/nim/large-language-models/latest/faq.html, but the current NVIDIA FAQ location is different and the available FAQ content does not establish that the API Catalog hosted endpoint used by this run is free to the caller. Current build.nvidia.com model pages do label prototype endpoints as free, while NIM offering/licensing documentation distinguishes free exploratory offerings from NVIDIA AI Enterprise production support. Record the exact reviewed page/version or immutable evidence artifact that supports the hosted endpoint claim; do not cite a generic or moved FAQ as proof.
4. MEDIUM — optional adapter boundary is lost
contextual_orchestrator.__init__ now imports the entire benchmark and monkeypatch installer for every package import. Keep the NIM evaluator optional: integrate the fix directly into its module, or install it only from the benchmark entrypoint. Runtime gateway users should not import benchmark-only code or mutate benchmark globals as a side effect of import contextual_orchestrator.
5. MEDIUM — temporary privileged workflow remains in the PR tree
.github/workflows/temporary-nim-transport-fix.yml has top-level contents: write, persists checkout credentials, and triggers on every branch push. It must be removed from the final tree. Any one-shot repair must use read-only defaults, isolate credentials to the publication step, validate exact changed paths, and disappear before review.
Keep the PR Draft. Replace the compatibility installer with direct, test-first implementation; remove the temporary workflow; update the PR body to the actual head; then rerun all exact-head checks and independent review.
|
Exact-head maintainer follow-up for the new hardening commits:
Do not resolve the security thread or mark the PR ready until these are implemented and verified on the exact repaired head. |
2 participants
Purpose
Implement issue #86 as a provider-neutral, evidence-grade NVIDIA NIM discovery and benchmark harness while preserving standalone operation and the modular CWL MSA boundary.
Exact integration identity
26f8d8dc5634f0371fad0801056e9a3450c78bfffix/atheris-interpreter-lock95a67eefa7578b19d032043793334a033ada7ed82df63e080737d5aa29d40871af99aa13411c3d76; it is not treated as the current stacked-base tip.The base branch is independently maintained and may move. Before every mutation and acceptance decision, live GitHub PR/ref metadata is authoritative. Any check, status, review, approval, mergeability result, or synthetic merge associated with another head/base pair is historical only.
Current-head verification state
For exact contributor head
26f8d8dc5634f0371fad0801056e9a3450c78bff:31182517889: success;31182517906: success;31182518012: success;Absent evidence is not success. One valid transactional-publication review thread remains intentionally unresolved until its evidence-gated acceptance condition is satisfied. The implementation response on that thread records test-first staging, complete-set validation, same-filesystem publication, rollback/recovery, and cleanup; the thread is not resolved merely because local tests succeeded.
The stacked base has advanced after this branch's earlier reconciliation, so no historical mergeability/check evidence is reused. After PR #96 integrates, this PR must be reconciled or retargeted from the exact integrated protected head and every required gate must be regenerated.
Implemented benchmark scope
GET /v1/modelscatalog without a hard-coded authoritative inventory;route_once, boundedconduct, and an optional reviewed cheapest-worker scenario under common total token and call envelopes;unknown;Strict scoring validity boundary
A test-first buyer-visible slice repairs construct-irrelevant score inflation in the locked task set.
337356117ede1b69ba1b47282038bfa0fcc9c360required complete-answer scorers that did not yet exist.exact_number_matchversion2accepts only one complete finite decimal response; prose, negation, units, multiple values,NaN, and infinities do not earn credit.exact_text_matchversion1compares the complete NFC-normalized, whitespace-normalized, case-folded response against explicit declared alternatives; substrings and undeclared aliases do not match.import contextual_orchestratordoes not import the benchmark, CSV adapter, or strict-scoring adapter;An isolated networkless harness exercised all 142 strict-scoring production statements and 54 branches and passed; that diagnostic validates the bounded module logic but is not repository CI, trusted central coverage, or merge evidence. Authoritative doctoring remains in
docs/doctoring/nim-benchmark-strict-answer-scoring.md.Provider and credential security boundary
NVIDIA_NIM_API_KEY; only the bounded live benchmark step receives the GitHub Secret;COPILOT_GITHUB_TOKENpath is introduced;Central dependencies — read-only
Central
.github#759is closed without merge at exact headdcb87ea3421ec87ce6acc62b04beef56758aa33e; none of its predecessor evidence transfers.Protected central
.githubmainis currently exact tip1131b1bbafb24e455fc8619cdf316813e8721861.The active protected-main prerequisites are owned exclusively by the central maintenance path:
.github#812: Draft at exact head2137012a2bf57b19a36f20ff6dcaac3e3e3b8cba, with the attempt-scoped coverage-artifact recovery contract still not accepted on protected main;.github#827: Draft from current protected-main base1131b1bbafb24e455fc8619cdf316813e8721861, exact headedddecdfa2b66b61f472db8f754270ffc2fd147c; it replaces the stale LLVM repair path and its exact-head Rust coverage-toolchain quality gate is failed, so the LLVM prerequisite is neither integrated nor green.Historical
.github#816evidence is superseded by the current-main replacement path and is not authority for this branch.This repository neither writes those central branches nor dispatches repair agents there. PR #96 must remain Draft until the required central changes reach protected
.githubmain, then regenerate fresh exact-head trusted central coverage/review evidence and obtain qualifying independent approval before protected merge. This PR must subsequently reconcile against the integrated #96 result and rerun every gate on the exact resulting head/base pair.Evidence contracts
Permanent regressions cover complete-plan admission, one-request-short zero-partial egress, deterministic bounded concurrency, valid media fixtures, DNS rebinding and non-global egress, redirects, proxy and credential isolation, bounded provider bodies, equal token/call budgets, reviewed pricing expiry, strict scoring, complete assignment evidence, Pareto exclusion labels, schema completeness, transactional publication, rollback/crash recovery, package import isolation, exact contributor-head workflow checkout, fuzz instrumentation, and buyer-readable evidence-status semantics.
Stack order and merge acceptance
.githubcoverage/review repairs, currently represented by #812 and #827, must reach protected centralmainunder their owning maintenance path;Queued, pending, skipped-required, cancelled, absent, stale-head, predecessor-head, failed, status-only, author-only, rate-limited, or synthetic-merge evidence is never success.
Refs #86.
Depends on #96 and the protected-main central repair path currently represented by ContextualWisdomLab/.github#812 and #827.